Skip to content

refactor(eval): address on-demand review feedback - #2007

Merged
jariy17 merged 3 commits into
aws:refactorfrom
jariy17:followup/eval-ondemand-review-feedback
Aug 18, 2026
Merged

refactor(eval): address on-demand review feedback#2007
jariy17 merged 3 commits into
aws:refactorfrom
jariy17:followup/eval-ondemand-review-feedback

Conversation

@jariy17

@jariy17 jariy17 commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Follow-up to #1983: makes on-demand evaluation failures easier to classify and removes review-identified noise.

Changes

  • Missing runtime telemetry now throws ResourceNotFoundError.
  • Failed CloudWatch queries now throw CloudWatchQueryError with query status and ID metadata.
  • Malformed telemetry produces at most one warning per command.
  • Repeated validation tests now use test.each.
  • Redundant and drift-prone comments were removed.

Decisions

  1. Fail fast: one session can require multiple Evaluate calls. Continuing after one fails could return incomplete results while appearing successful.
  2. Keep results flat: context.spanContext.sessionId supports per-session filtering without changing the SDK response shape.
  3. Defer a generic poller: extract one when another production caller needs the same behavior.
  4. Keep window resolvers separate: batch feeds the API's DataSourceConfig, including CloudWatch log groups and onlineEvaluationConfigArn; on-demand feeds client-side Logs Insights.

Verification

  • Build, typecheck, lint, changed-file formatting, and focused tests pass.
  • Full suite: 1,373 tests pass.

@github-actions github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 14, 2026
@codecov-commenter

codecov-commenter commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.13%. Comparing base (2e18071) to head (8ffdcb9).

Additional details and impacted files 
@@             Coverage Diff              @@
##           refactor    #2007      +/-   ##
============================================
+ Coverage     97.09%   97.13%   +0.04%     
============================================
  Files           381      381              
  Lines         22778    22786       +8     
============================================
+ Hits          22116    22134      +18     
+ Misses          662      652      -10

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 14, 2026
Hweinstock
Hweinstock reviewed Aug 15, 2026
View reviewed changes

@Hweinstock Hweinstock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for coming back to this! one question on the testing pattern, otherwise lgtm.

Comment thread src/handlers/eval/ondemand/ondemand.test.tsx
return { core, stdout: io.stdout(), stderr: io.stderr() };
}

async function runWithRealCore(options: LogsOptions, logger = createSilentLogger()) {

@Hweinstock Hweinstock Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this use the fixture pattern? Might be a little harder to trigger these cases, but seems possible unless i'm missing something.

@jariy17 jariy17 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept it on inline stubs on purpose. The fixture harness records against a live account; these cases can't be provoked there:

  1. Query resolving Failed/Cancelled/Timeout
  2. Runtime log group missing while aws/spans exists
  3. Malformed telemetry rows — this test also asserts logger warnings, which the fixture run() helper (silent logger, stdout only) can't capture

Same .send() seam, same real CoreClient, so it's the deterministic counterpart for the negative paths, not a weaker test. Added a comment saying so in ab60ff5.

@Hweinstock

Hweinstock commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

is this from this PR?


Run bun run format:check
$ prettier --check .
Checking formatting...
[warn] src/core/types.tsx
[warn] Code style issues found in the above file. Run Prettier with --write to fix.
error: script "format:check" exited with code 1

@jariy17

jariy17 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

is this from this PR?

Yes — dropped semicolon in src/core/types.tsx:40. Restored it in ab60ff5; bun run format:check is clean now.

jariy17 added 3 commits August 18, 2026 22:04
jariy17
refactor(eval): address on-demand review feedback
3762c65
jariy17
test(eval): consolidate on-demand regression coverage
1676291
jariy17
fix(eval): restore semicolon and document runWithRealCore rationale
8ffdcb9 
- Restore dropped semicolon in src/core/types.tsx that failed format:check
- Explain why runWithRealCore stubs the SDK .send() seam inline instead of
  using the golden fixture harness (failure conditions the recorder can't
  provoke against a live account)
@jariy17
jariy17 force-pushed the followup/eval-ondemand-review-feedback branch from ab60ff5 to 8ffdcb9 Compare August 18, 2026 22:06
@jariy17
jariy17 merged commit ee854eb into aws:refactor Aug 18, 2026
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hweinstock Hweinstock Hweinstock approved these changes
@aidandaly24 aidandaly24 aidandaly24 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jariy17 @codecov-commenter @Hweinstock @aidandaly24